Skip to content

GH-34094: [C++] Increase Boost minimum version for clang >= 16#34100

Merged
kou merged 6 commits into
apache:masterfrom
assignUser:boost-min-version-clang16
Feb 10, 2023
Merged

GH-34094: [C++] Increase Boost minimum version for clang >= 16#34100
kou merged 6 commits into
apache:masterfrom
assignUser:boost-min-version-clang16

Conversation

@assignUser

@assignUser assignUser commented Feb 9, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

Build fails with older Boost on Clang >= 16

Are there any user-facing changes?

No our dependency management will download the required Boost version if it is not installed on the system.

@assignUser assignUser marked this pull request as ready for review February 9, 2023 13:48
@github-actions

github-actions Bot commented Feb 9, 2023

Copy link
Copy Markdown

@thisisnic

thisisnic commented Feb 9, 2023

Copy link
Copy Markdown
Member

Thanks for updating this! Looks like there's a linter error which needs fixing, but aside from that, I tried this locally in a container, and it appears to be working - the Arrow C++ build now fails when being built using clang16 with Boost_SOURCE=SYSTEM and an older version of Boost, but uses the bundled version with Boost_SOURCE=AUTO or Boost_SOURCE=BUNDLED.

@assignUser

assignUser commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

now fails when being built using clang16 with Boost_SOURCE=SYSTEM and an older version of Boost, but uses the bundled version with Boost_SOURCE=AUTO or Boost_SOURCE=BUNDLED.

Nice, that's the expected result 🎉

nealrichardson
nealrichardson reviewed Feb 9, 2023
View reviewed changes
Comment thread cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated
westonpace
westonpace approved these changes Feb 9, 2023
View reviewed changes

@westonpace westonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't really been following this discussion but this change seems safe to me. If someone is using clang 16 then either they are on a very new OS (which probably has a more recent version of boost) or they are advanced enough they can get a newer boost and comfortable with using newer libs.

kou
kou reviewed Feb 9, 2023
View reviewed changes
Comment thread cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated
kou and others added 2 commits February 10, 2023 12:14
@kou @nealrichardson
Use VERSION_GREATER
ff86840 
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
@kou
Fix style
24ffa10 
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
kou
kou approved these changes Feb 10, 2023
View reviewed changes

@kou kou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou kou merged commit 717d4fb into apache:master Feb 10, 2023
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Feb 10, 2023
@assignUser @kou
@nealrichardson @thisisnic
apacheGH-34094: [C++] Increase Boost minimum version for clang >= 16 (a…
be93ef0 
…pache#34100)

### Rationale for this change

Build fails with older Boost on Clang >= 16

### Are there any user-facing changes?
No our dependency management will download the required Boost version if it is not installed on the system.
* Closes: apache#34094

Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@ursabot

ursabot commented Feb 10, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 106568d and contender = 717d4fb. 717d4fb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.37% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.79% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.35% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 717d4fb7 ec2-t3-xlarge-us-east-2
[Failed] 717d4fb7 test-mac-arm
[Finished] 717d4fb7 ursa-i9-9960x
[Finished] 717d4fb7 ursa-thinkcentre-m75q
[Finished] 106568d5 ec2-t3-xlarge-us-east-2
[Finished] 106568d5 test-mac-arm
[Finished] 106568d5 ursa-i9-9960x
[Finished] 106568d5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented Feb 10, 2023

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nealrichardson nealrichardson nealrichardson left review comments
@kou kou kou approved these changes
@westonpace westonpace westonpace approved these changes
@thisisnic thisisnic thisisnic approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Boost minimum version needs increasing for clang16 builds

6 participants

@assignUser @thisisnic @ursabot @kou @westonpace @nealrichardson